Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix __geo_interface__ if parts or points is empty list #101

Closed
wants to merge 1 commit into from

Conversation

Tisp
Copy link

@Tisp Tisp commented Jul 6, 2017

Fixes UnboundLocalError when shapes have empty parts or points

File "/usr/local/lib/python2.7/site-packages/shapefile.py", line 174, in __geo_interface__ coordinates.append(tuple([tuple(p) for p in self.points[part:]])) UnboundLocalError: local variable 'part' referenced before assignment


""" Case parts or points is empty """
if not self.parts or not self.points:
return False
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

False? I am not sure about this. how does geojson handle empty geometries?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

False is a valid value for geojson data structure. (https://geojson.org/geojson-spec.html#geometry-objects) I did some tests with geojson that contains a false value in geometry fields in GIS tools, and i not found erros.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There doesn't seem to be an empty geometry type defined in GeoJSON, but it does state that feature objects without a geometry can be specified with JSON null value, which would be None rather than False in Python.

From the previous link:

A feature object must have a member with the name "geometry". The value of the geometry member is a geometry object as defined above or a JSON null value.

I also think None makes more sense since there literally is "none" geometry object.

@karimbahgat
Copy link
Collaborator

Thanks for this @Tisp. However, I believe the unbound local error happens for good reasons.

Having geo_interface return False or None would imply that this is a null-geometry. But geo_interface is already built to return None when the shape type is set to NULL. This means your code was processing a real geometry, specifically a polyline or polygon geometry which should always contain a parts list with at least one 0-value ([0]). The real problem here is that the parts list was instead empty. Returning a JSON null for such cases would misleadingly suggest this is a null geometry and also hide the fact that the shape object was not correctly created.

So while the error message is being raised correctly it is not very informative. Perhaps instead of returning False we can return a more informative exception if the parts attribute is empty? Furthermore, this should only be checked for polyline or polygon types, as these are the only ones expected to have the parts attribute.

@dbstovall
Copy link

I agree. Right now I'm handling the UnboundLocalError but it seems like it should be something like an InvalidShapeError.

@karimbahgat
Copy link
Collaborator

Fixed this now, raises a generic Exception when this happens (but see #146). Also raises error if shape type is beyond what the GeoJSON specification can represent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants